Transition to salsa coarse-grained tracked structs #15763
Conversation
crates/red_knot_python_semantic/src/module_resolver/resolver.rs
Outdated
Show resolved
Hide resolved
ibraheemdev
force-pushed
the
salsa-coarse-deps
branch
from
6a70801 to
184a795
Compare
CodSpeed Performance ReportMerging #15763 will improve performances by 14.08%Comparing Summary
Benchmarks breakdown
|
ibraheemdev
force-pushed
the
salsa-coarse-deps
branch
from
184a795 to
e6badd1
Compare
|
ibraheemdev
force-pushed
the
salsa-coarse-deps
branch
3 times, most recently
from
f6f1bd8 to
21bea37
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
| @@ -42,7 +40,6 @@ pub(crate) struct PatternConstraint<'db> { | |||
| #[return_ref] | |||
| pub(crate) kind: PatternConstraintKind<'db>, | |||
|
|
|||
| #[no_eq] | |||
There was a problem hiding this comment.
Countme's eq implementation is a no-op anyway
MichaReiser
force-pushed
the
salsa-coarse-deps
branch
from
c445b8f to
d34846b
Compare
| pub(crate) file_scope: FileScopeId, | ||
|
|
||
| #[no_eq] | ||
| #[return_ref] |
There was a problem hiding this comment.
Expression is Copy, no need to return a ref
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
MichaReiser
force-pushed
the
salsa-coarse-deps
branch
from
5106cec to
1dfe37d
Compare
|
I updated the salsa dep to use the upstream commit and I'll mark it ready for review. It might be good to get a review from someone other than me, considering that I did some of the work but I think it's also fine if it's just me reviewing it if everyone else is short on time. I'll merge the PR on Wednesday if it hasn't received any reviews by then. |
MichaReiser
requested review from
carljm,
AlexWaygood and
sharkdp
as code owners
There was a problem hiding this comment.
I'm okay with removing this module for now per the conversation we had in #15834, but it seems unrelated to the PR?
There was a problem hiding this comment.
It is related in the sense that the upgrade requires changes to that module and I decided that it's not worth my time updating unused code.
There was a problem hiding this comment.
this is enough to make everything compile without warnings, FWIW :P
diff --git a/crates/red_knot_python_semantic/src/types/statistics.rs b/crates/red_knot_python_semantic/src/types/statistics.rs
index 625a38d52..561765dd3 100644
--- a/crates/red_knot_python_semantic/src/types/statistics.rs
+++ b/crates/red_knot_python_semantic/src/types/statistics.rs
@@ -1,11 +1,12 @@
+#![allow(unused)]
+
use crate::types::{infer_scope_types, semantic_index, Type};
use crate::Db;
use ruff_db::files::File;
use rustc_hash::FxHashMap;
/// Get type-coverage statistics for a file.
-#[salsa::tracked(return_ref)]
-pub fn type_statistics<'db>(db: &'db dyn Db, file: File) -> TypeStatistics<'db> {
+pub(crate) fn type_statistics(db: &dyn Db, file: File) -> TypeStatistics {
let _span = tracing::trace_span!("type_statistics", file=?file.path(db)).entered();
tracing::debug!(
@@ -71,7 +72,7 @@ mod tests {
db: &'db mut TestDb,
filename: &str,
source: &str,
- ) -> &'db TypeStatistics<'db> {
+ ) -> TypeStatistics<'db> {
db.write_dedented(filename, source).unwrap();
| fn eq(&self, other: &Self) -> bool { | ||
| self.node().eq(other.node()) | ||
| self.node.eq(&other.node) |
There was a problem hiding this comment.
What is the relationship of this change to the rest of the PR?
Could we do this today in main, independently of the tracked-struct changes? And if we did, how much of the perf win of this PR would come with it?
(Not really suggesting we need to split it out, just trying to understand this change and its perf impact better.)
There was a problem hiding this comment.
We could probably revert this change now that AstNodeRef fields are tracked again. Let me try tomorrow.
I don't think changing the implementation on main would lead to a performance improvement because all AstNodeRef fields are marked with no_eq and are never marked as id (they're never hashed). That means this implementation should be mostly unused today.
There was a problem hiding this comment.
It looks like you decided not to revert this change? Anything worth documenting about why?
…Eq` or `Hash` (#16100) ## Summary This is a follow up to #15763 (comment) It reverts the change to using ptr equality for `AstNodeRef`s, which in turn removes the `Eq`, `PartialEq`, and `Hash` implementations for `AstNodeRef`s parametrized with AST nodes. Cheap comparisons shouldn't be needed because the node field is generally marked as `[#tracked]` and `#[no_eq]` and removing the implementations even enforces that those attributes are set on all `AstNodeRef` fields (which is good). The only downside this has is that we technically wouldn't have to mark the `Unpack::target` as `#[tracked]` because the `target` field is accessed in every query accepting `Unpack` as an argument. Overall, enforcing the use of `#[tracked]` seems like a good trade off, espacially considering that it's very likely that we'd probably forget to mark the `Unpack::target` field as tracked if we add a new `Unpack` query that doesn't access the target. ## Test Plan `cargo test`
* main: add diagnostic `Span` (couples `File` and `TextRange`) (#16101) Remove `Hash` and `Eq` from `AstNodeRef` for types not implementing `Eq` or `Hash` (#16100) Fix release build warning about unused todo type message (#16102) [`pydocstyle`] Handle arguments with the same names as sections (`D417`) (#16011) [red-knot] Reduce usage of `From<Type>` implementations when working with `Symbol`s (#16076) Transition to salsa coarse-grained tracked structs (#15763) [`pyupgrade`] Handle micro version numbers correctly (`UP036`) (#16091) [red-knot] `T | object == object` (#16088) [`ruff`] Skip singleton starred expressions for `incorrectly-parenthesized-tuple-in-subscript` (`RUF031`) (#16083) Delete left-over `verbosity.rs (#16081) [red-knot] User-level configuration (#16021) Add `user_configuration_directory` to `System` (#16020)
Summary
Transition to using coarse-grained tracked structs (depends on salsa-rs/salsa#657). For now, this PR doesn't add any
#[tracked]fields, meaning that any changes cause the entire struct to be invalidated. It also changesAstNodeRefto be compared/hashed by pointer address, instead of performing a deep AST comparison.Test Plan
This yields a 10-15% improvement on my machine (though weirdly some runs were 5-10% without being flagged as inconsistent by criterion, is there some non-determinism involved?). It's possible that some of this is unrelated, I'll try applying the patch to the current salsa version to make sure.